[6.7] Zoom in/out to correct location #66618
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeryj
added
[Feature] Zoom Out
[Type] Bug
|
Size Change: +291 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0b4d86d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11784136135
|
2 tasks
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ajlende
reviewed
Nov 8, 2024
This was referenced Nov 13, 2024
|
What happened to preserving the correct position and ratio when you open the inserter? I'm disoriented whenever I open the inserter:
|
|
Is this really something we need in a point release? It seems a bit risky tbh. |
|
But this works well when I use the dedicated zoom-out button 👍 cool! |
ellatrix
reviewed
Nov 18, 2024
| $scroll-top: var(--wp-block-editor-iframe-zoom-out-scroll-top, 0); | ||
| $scroll-top-next: var(--wp-block-editor-iframe-zoom-out-scroll-top-next, 0); | ||
|
|
||
| position: fixed; |
There was a problem hiding this comment.
Could you comment on why fixed positioning is needed on the html element to make this work?
ellatrix
reviewed
Nov 18, 2024
| // ); | ||
| // iframeDocument.documentElement.style.removeProperty( | ||
| // '--wp-block-editor-iframe-zoom-out-scale-container-width' | ||
| // ); |
There was a problem hiding this comment.
Seems fine. So we always want to calculate these variables I guess. Unmounting is not needed then because when the iframe unmounts these attributes are automatically removed too.
ellatrix
approved these changes
Nov 18, 2024
There was a problem hiding this comment.
Ok, so you're pre-calculating the final scroll position at the start of the animation, then you animate it in CSS in translate as an offset, and finally when the transition ends, you swap that out for a real scroll position? That's brilliant!
There's some similarities with the block moving animation, but there we set the scroll position continuously, which is probably not so smooth in this case. See:
It would be nice if we could move all this logic to a separate hook outside the iframe component.
|
Super nit pick: when I exit zoom out and I'm all the way at the bottom, it seems the scroll position doesn't stay at the bottom. But that's fine if it's too difficult. |
|
Btw, I approved this for trunk, I am not sure if this should be merged into 6.7. I'll leave that up to the release leads. I didn't see that this was targeting the 6.7 immediately. |
getdave
added
the
Backport to WP Minor Release
|
Pinging @cbravobernal who is leading 6.7.1 from Core Editor side. |
|
Maybe rebase this onto trunk and then we can cherry pick if needed? |
|
I have a |
We had some code to do this at one point, but after implementing it, we realized that it also felt off to zoom in to the bottom, as you may be working with the central part of the canvas. Either way it feels a little off, so we opted to go with the simpler method. |
@ellatrix I believe we only preserve the ratio when you start from zoom out. I think without zoom out engaged the sidebars have always caused reflow. |
ajlende
changed the title
|
Let's include it in 6.7.2 |
|
Where is the PR for trunk? |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #65884
Also fixes a minor bug where setting the scroll position was still delayed even when there was no animation due to reduced motion settings.
What?
Maintain the visible content of the editor canvas when zooming in/out.
Why?
It's disorienting to zoom in/out and lose your frame of reference.
How?
When changing scale (zoom in/out):
It will always attempt to scale to the center unless we are at the top of the canvas. In which case, it will scale to keep the top of the canvas visible.
Known bugs
Testing Instructions
Zooming from Top
Screen.Recording.2024-11-06.at.2.38.52.PM.mov
Zooming from below Top
Screen.Recording.2024-11-06.at.2.39.17.PM.mov
Open/close sidebar scaling animations
Try to break it :)
Testing Instructions for Keyboard
Screenshots or screencast
Before
Screen.Recording.2024-11-06.at.3.29.26.PM.mov
After
Screen.Recording.2024-11-06.at.12.27.31.PM.mov